Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronize EIP-4844 cryptography with consensus specs #5649

Merged
merged 1 commit into from
Oct 3, 2022

Conversation

asn-d6
Copy link
Contributor

@asn-d6 asn-d6 commented Sep 12, 2022

So... at some point back in June we updated the consensus side of 4844 to be executable. This resulted in various fixes and improvements for the cryptographic parts of the consensus code.

These updates were never ported back to EIP land because the code was quite unstable and we wanted to minimize any duplicate overhead between EIP and consensus review processes. This means that the EIP was out of date for the past months...

These days, the 4844 cryptographic code seems relatively stable, so this commit updates the EIP. Also, with the merge coming up, more devs will start looking at 4844 code, and hence having the code be up to date is imperative.

Note that instead of updating the cryptographic functions we instead link to the relevant parts of the consensus-specs
repository. This is done to avoid code duplication between the two repositories -- an approach that seems prudent in
cross-execution-and-consensus situations like EIP-4844. I imagine that people might not like this approach, but in that case, I would appreciate feedback on alternative approaches we could take.

Cheers!

@github-actions github-actions bot added c-update Modifies an existing proposal s-draft This EIP is a Draft t-core labels Sep 12, 2022
@asn-d6 asn-d6 marked this pull request as draft September 12, 2022 08:01
@eth-bot
Copy link
Collaborator

eth-bot commented Sep 12, 2022

All tests passed; auto-merging...

(pass) eip-4844.md

classification
updateEIP
  • passed!

@lightclient
Copy link
Member

@asn-d6 could you update the consensus-specs links so that they link to a specific release/revision of the spec?
@SamWilsn what is the status of allowing consensus-specs links? Are we just waiting to update the eipw or is it still under discussion?

@asn-d6
Copy link
Contributor Author

asn-d6 commented Sep 14, 2022

@asn-d6 could you update the consensus-specs links so that they link to a specific release/revision of the spec?

I was thinking that part of the appeal of this approach is that the link will always point to the latest version of the functions, and as long as the function names remain the same we will not have to change the EIP-side at all. Do you think that's too flexible?

@asn-d6
Copy link
Contributor Author

asn-d6 commented Sep 26, 2022

@asn-d6 could you update the consensus-specs links so that they link to a specific release/revision of the spec? @SamWilsn what is the status of allowing consensus-specs links? Are we just waiting to update the eipw or is it still under discussion?

Thinking about this again, I think doing it one step at a time is fine.

I have force-pushed a new commit 851d380, that pins down a specific revision (current consensus-specs HEAD) for all functions cited. This should prevent any sort of rug pulling from the consensus-specs side.

This took a while because I wanted to make sure that all hovering EIP4844 crypto-related changeshave been merged to consensus-specs, which is now the case.

Please let me know what else I can do on this one :)

This commit introduces various fixes and improvements to the KZG-related code of EIP-4844. This commit brings the EIP
code up-to-speed with the consensus-specs code.

Note that instead of updating the cryptographic functions we instead link to the relevant parts of the consensus-specs
repository. This is done to avoid code duplication between the two repositories -- an approach that seems prudent in
cross-execution-and-consensus situations like EIP-4844.
@lightclient
Copy link
Member

Hey @SamWilsn is it okay to manually merge while we wait for the walidator to accept the CL spec links, or are we still trying to come to agreement on that?

Copy link
Contributor

@Inphi Inphi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to maintainers on this whether it's appropriate to link to the consensus specs.
Though my 2c is that EIPs should be as self contained as possible, such that implementers can use as a reference without consulting another documents.

@asn-d6 asn-d6 marked this pull request as ready for review October 3, 2022 15:40
@eth-bot eth-bot enabled auto-merge (squash) October 3, 2022 15:41
@lightclient
Copy link
Member

lightclient commented Oct 3, 2022

I'm going to override eipw here as it's been 3 weeks and have not got an update on when eipw will support links to the consensus specs.

The editors are still debating the specifics, but it appears that links to the consensus specs which refer to a specific commit will be acceptable. In order to avoid any further confusion among implementation teams currently working on EIP-4844, I'm going to merge these changes as I have verified that each link is to a specific revision of the spec. We can continue debating the exact external link policy and we will review the current state of things as this EIP moves forward to review.

@lightclient lightclient merged commit ae32e77 into ethereum:master Oct 3, 2022
nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
This commit introduces various fixes and improvements to the KZG-related code of EIP-4844. This commit brings the EIP
code up-to-speed with the consensus-specs code.

Note that instead of updating the cryptographic functions we instead link to the relevant parts of the consensus-specs
repository. This is done to avoid code duplication between the two repositories -- an approach that seems prudent in
cross-execution-and-consensus situations like EIP-4844.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-update Modifies an existing proposal s-draft This EIP is a Draft t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants